-
Notifications
You must be signed in to change notification settings - Fork 2.7k
Added token revocation functionality to Managed Identity's App Service and Service Fabric sources #7679
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: dev
Are you sure you want to change the base?
Conversation
lib/msal-node/test/client/ManagedIdentitySources/AppService.spec.ts
Outdated
Show resolved
Hide resolved
lib/msal-node/test/client/ManagedIdentitySources/AppService.spec.ts
Outdated
Show resolved
Hide resolved
lib/msal-node/test/client/ManagedIdentitySources/AppService.spec.ts
Outdated
Show resolved
Hide resolved
@@ -176,6 +178,70 @@ describe("Acquires a token successfully via an App Service Managed Identity", () | |||
}); | |||
}); | |||
|
|||
describe("Miscellaneous", () => { | |||
test("ignores a cached token when claims are provided and the Managed Identity does support token revocation, and ensures the token revocation query parameter token_sha256_to_refresh was included in the network request to the Managed Identity", async () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add test when just the capabilities are set and claims are not passed?
} from "../../utils/Constants.js"; | ||
import { CryptoProvider } from "../../crypto/CryptoProvider.js"; | ||
import { ManagedIdentityRequestParameters } from "../../config/ManagedIdentityRequestParameters.js"; | ||
import { ManagedIdentityId } from "../../config/ManagedIdentityId.js"; | ||
import { NodeStorage } from "../../cache/NodeStorage.js"; | ||
|
||
// MSI Constants. Docs for MSI are available here https://docs.microsoft.com/azure/app-service/overview-managed-identity | ||
const APP_SERVICE_MSI_API_VERSION: string = "2019-08-01"; | ||
const APP_SERVICE_MSI_API_VERSION: string = "2025-03-30"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please do not merge this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alternatively, remove the app service changes from this PR, and then we can merge it for Service Fabric. We will create a new PR later after app service deploys their token revocation support.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gladjohn Expected GA for App Service token revocation is 5/30? I will wait to merge until then if you can confirm.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Expected GA for App Service token revocation is 5/30?
5/30 is availability of the feature in test ring.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall, the PR looks solid. It adds a clear, consistent approach for token revocation. If you remove 'app service' the PR can be merged. Else,. please mark it as 'Do Not Merge'
Token Revocation spec